NEW @W-17875752@ Added unused temporary directories - #337

A few things.

First I wonder the performance implications of using rm at this time vs what we have today using the tmp library. Note that the tmp library does some funny things I've found with cleanup (tied to the node process's teardown hooks or whatever) and I recall experiencing a major slowdown when I attempted to delete temp folders that it created manually with a "rm" in the past. So please test this against the current code with NPSP or something to see if we can safely move directly to using deleteDirectory here or not.

Second, we should put this into a separate try catch and I wouldn't error if it can't delete the directory for whatever reason but instead just log the error event without making tied to the uninstantiable engine event stuff... because clearly the engine did instantiate and work just fine.

Third, do we want the creation of the directory to auto clean itself up (sort of like with a finally block)... unless we decide that we want to keep it (due to specific types of errors or whatever)? Doesn't need to be done now, but I just wonder if we want this delete to be complete skipped on any exception? (maybe?? just thinking out loud)

• I'll do some testing with a big repo to see if there's a performance impact from switching to rm.
• Okay, I can use a boolean value to determine whether things succeeded and then delete the directory that way instead of doing it within this try-catch itself.
• My assumption was to make the deleteDirectory call in getAllRules only happen if that directory was empty (meaning that every engine succeeded and deleted its temp folder), but we're not there yet.

Can we please call it "workingFolder" instead of workingDirectory. 2 reasons: 1) not to be confused with the current working directory (CWD) and 2) because it matches what the field should be in the run options.

I'll change to workingFolder and tempFolder instead of workingDirectory and tempDirectory.

this is supposed to be called workingFolder and not workingDirectory.

My bad. Not sure where I got the name workingDirectory, but I'll change it.

I thought the idea was to just for now lift the tmp dependency (by re-using the existing engine utility's way of creating a temp directory) so that we don't accidentally change behavior.

If we switch to using mkdir and rm manually... we'll need to do a performance analysis.

I'll need to check how tightly we can control tmp's decision to preserve or delete the directory, but yeah I can try this.

We shouldn't be doing a delete at this level. We should only be doing a delete at the run-<timestamp> and rules-<timestamp> level and not at the engine level for simplicity. Again the idea of doing the delete ourselves was going to be deferred to another work item after evaluation alternative options to tmp I thought. The easiest thing to do right now is just to keep using that createTempDir util that you added which still uses the tmp library.

Why wouldn't we do it this way? The way I have it, the engine's folder is only persisted if its execution fails, and then we can have the entire root folder be deleted at the end if it no longer has any contents.

I know you made this a no-op but can we please just remove this code. A part of my earlier feedback was that failure to delete a folder shouldn't be treated as an unexpectedErrorEngineRunResult anyway.

I'd prefer to just delete this right now and we'll put it back in if/when we need it

This all seems overly complicated and unneeded. Also that await call in a loop with existsSync checks will be rather slow.

Why not just use the createTempDir one time (our existing util on the engine api) and store it on the Code Analyzer instance as the root temp folder we will be using. (using a getter that checks to see if it has been created yet)

Then underneath that temp folder then you use your createDirectory stuff to just create the subfolders underneath it. We shouldn't even need a createNamedTempDir stuff. For example:

First time you need the root level temp folder (tied to the CodeAnalyzer instance) you create it with with createTempDir. Then under this folder, when we fetch from the engines all the rule descriptions, we create a subfolder within that root temp folder the rules-<timestamp> folder and under that folder you create each <engine_name> folder.

And when someone calls run, again we get the root temp folder (either from the cache or fresh) and then create a run-<timestamp> subfolder which then will contain <engine_name> subfolders.

No need to do all this checking if things exists or not, because you control the top level folder and always create in the proper order.

<code-analyzer-instance-level-root-temp-dir> <-- only need to create 1 top level temp directory for now

• rules-<timestamp>
  • pmd
  • eslint
  • etc
• run-<timestamp>
  • pmd
  • eslint
  • etc

And again for now the cleanup will be automatically done by the tmp graceful cleanup stuff at the code analyzer instance level. In the future, we'll decide at the rules or run level how we want to preserve folders.

Reasonable. I'll look at simplifying this.

Still pretty sure I need the createNamedTempDir stuff in order to explicitly name the engine-level temp folders after the engine instead of them having a randomly assigned name. Otherwise I'll have to futz with the signature of the existing createTempDir function, which I'm trying to avoid.

We shouldn't be hard coding os.tmpdir in our test at all. Can't we just spy on the root folder being created and validate it was created and just use its value for the remainder of assertions?

We can, if we want, do equality checks on the workspace and logfolder properties, and then do some spying.
But what's wrong with hardcoding the os.tmpdir in here? It's where the folder is supposed to go. Once it's configurable, we'll have test coverage for configuring it too.

Because you are locking in implementation detail. It isn't a part of the contract. The test needs to only fail if the contract fails. If we decide to change the location of where the working folder is (and it happens to not live in os.tmpdir) then no test should fail. Basically, I don't think it is a requirement to lock in with a test that a folder must live under os.tmpdir. It is only a requirement that we give a workingFolder to the engine.

If you write your test first that you should receive in the runOptions and the describeOptions a workingFolder field that has have some folder that exists that ends with ${path.sep()}${engineName} then that is all we need for a test because that is all we are saying about the contract.

Why do we need this?

Everytime we call tmpDirAsync we are having tmpDirAsync keep track of more things.

Once the root instance level temp directory is created from the createTempDir then we should easily be able to just do a mkdir to create subfolders.